Fix opensearch theme version #978

zuochengding · 2021-11-24T01:59:37Z

Signed-off-by: Zuocheng Ding [email protected]

Description

This PR will update the old kibana theme version and tag to v1 and v2.

[EDIT] : The new revision will change the default theme version to v1 and remove the beta theme

Issues Resolved

[BUG] Theme version references v7 and v8 #494

Check List

kavilla · 2021-11-24T02:52:07Z

Nit: Can we update the commit to include more details and have a description. Doesn't have to be much and you can add the issue in there as well.

kavilla · 2021-11-24T18:39:30Z

For tracking purposes related to this PR. Can we say we manually verified this will not break backwards compatibility?

zuochengding · 2021-11-24T18:51:22Z

For tracking purposes related to this PR. Can we say we manually verified this will not break backwards compatibility?

Yes, the PR has no logical change but just updates the naming of the themes. This will only affect the UI. From my local testing, there is no behavior change. In theory, there shouldn't have any regression issue. However, one thing I have to mention here is that v2 (or the original v8(beta)) has some legacy SCSS compiling errors in console.

ananzh

LGTM

tmarkley · 2021-11-29T19:26:20Z

I think we should consider just using the default and removing this option as Jules mentioned in #494.

jcgraybill · 2021-11-30T19:31:47Z

Yes, let's get @ahopp to take a look at this before merging - to weigh in on what to call these two alternative themes, or whether to just standardize on a single theme.

ahopp · 2021-12-09T18:01:11Z

@tmarkley sorry for the delay! I'm not seeing a ton of value in the "secondary" theme. For now, I think it would be better to standardize on a single theme. Do we have a good view of the differences between the two themes?

zuochengding · 2021-12-09T18:56:06Z

@tmarkley sorry for the delay! I'm not seeing a ton of value in the "secondary" theme. For now, I think it would be better to standardize on a single theme. Do we have a good view of the differences between the two themes?

The theme (v8 beta) does't seem fully finished, I have noticed some layout cutting in UI (and errors in console as well) for the second theme (v8 beta). From this standing point, i think it is good to remove.

ahopp · 2021-12-09T19:36:51Z

Roger, in that case it makes sense to align on the "v7" and begin any work on new theming can be resolved at a latter date. Thanks!

kavilla · 2021-12-13T10:03:10Z

If any case, if we remove the option lets make sure it doesn't break any backwards compatibility on start up if someone from the legacy app had this setting to the beta.

boktorbb

I agree with backwards compatibility verification; otherwise, looks good to me

src/core/server/ui_settings/settings/theme.test.ts

packages/osd-ui-shared-deps/theme.ts

zuochengding · 2021-12-14T20:54:14Z

If any case, if we remove the option lets make sure it doesn't break any backwards compatibility on start up if someone from the legacy app had this setting to the beta.

Thanks @ananzh for helping me on backwards compatible testing from " odfe-1.13.2 vanilla version" to the "latest change". The UI looks good to me.

zuochengding · 2022-01-11T23:53:22Z

@zuochengding are we targeting v2.0.0 for this change? Can we add tests that confirm that the v1 theme is properly set when either v7 or the v8 beta theme were selected previously?

Yes, we are targeting v2.0.0 and I will add more tests in next revision

Originally, in Advanced setting, there is a setting let you set theme version from `v7` to `v8(beta)` implying version that do not exist yet. Change the version label to `v1` as default theme and remove the beta version. Hide `theme:version` UI from Advanced Setting since we have only one theme avaialble now. Signed-off-by: Zuocheng Ding <[email protected]>

tmarkley

I think before we merge this we should probably have integration and functional tests that confirm that no matter what setting was applied before updating, v1 is always applied.

tmarkley · 2022-01-14T17:17:14Z

src/core/server/ui_settings/settings/theme.ts

+        schema.literal('v7'),
+        schema.literal('v8 (beta)'),


Why can't we remove these from the schema?

This is for backward compatibility, user's injectedMetadata from previous version is readonly and there is no way to modify it. The current system will do an initial schema validation during the startup, so you will see some warnings in console if we remove this fields. The way I proposed is to override the user injectedData after the data loaded (deep copied) into the memory (cache), then I override it as default value v1.

tmarkley · 2022-01-14T17:18:03Z

src/core/server/ui_settings/settings/theme.test.ts

-      expect(() => validate('v7')).not.toThrow();
-      expect(() => validate('v8 (beta)')).not.toThrow();


Can we keep these and expect them to throw?

same reason as above, It will be validated based on the schema we defined

zuochengding · 2022-01-14T18:49:33Z

I think before we merge this we should probably have integration and functional tests that confirm that no matter what setting was applied before updating, v1 is always applied.

I think the unit test might be sufficient because it covered the main concern for naming change. Just to make sure we are on same page, v1 isn't something new but just a renamed theme of v7. Compared with previous, only one theme will be loaded no matter what name it is. The only concern would be what is name will displayed to customer. The unit test will verify the override . Functional testing is something we could do but since we actually disabled/hided the from the Advanced Setting, not very sure if the value can be retrieved from UI level verification. I don't see much benefit for this since it is covered in unit test

kavilla · 2022-02-07T17:29:36Z

@zuochengding, @tmarkley, in summation what would be the last things to get this PR in?

tmarkley · 2022-02-07T17:37:43Z

src/core/public/ui_settings/ui_settings_client.ts

@@ -175,6 +177,17 @@ You can use \`IUiSettingsClient.get("${key}", defaultValue)\`, which will just r
    return this.updateErrors$.asObservable();
  }

+  resetThemeVersion() {
+    const key = 'theme:version';
+    // Reset userValue to default value if any legacy theme verion exists.


minor: typo verion should be version

tmarkley · 2022-02-07T17:39:20Z

src/core/public/ui_settings/ui_settings_client.ts

+    const key = 'theme:version';
+    // Reset userValue to default value if any legacy theme verion exists.
+    if (
+      this.cache[key] !== undefined &&


Shouldn't we set the value if the cache is undefined?

tmarkley · 2022-02-07T17:42:01Z

I think before we merge this we should probably have integration and functional tests that confirm that no matter what setting was applied before updating, v1 is always applied.

I think the unit test might be sufficient because it covered the main concern for naming change. Just to make sure we are on same page, v1 isn't something new but just a renamed theme of v7. Compared with previous, only one theme will be loaded no matter what name it is. The only concern would be what is name will displayed to customer. The unit test will verify the override . Functional testing is something we could do but since we actually disabled/hided the from the Advanced Setting, not very sure if the value can be retrieved from UI level verification. I don't see much benefit for this since it is covered in unit test

Apologies for not responding to this sooner. The unit test covers the functionality of resetThemeVersion, but we don't have anything that confirms whether that function is called in all scenarios and properly sets the theme for all users.

kavilla · 2022-04-14T00:45:30Z

There's a lot of conflicts here and I think the PR still as to address changes. Should we remove the 2.0.0 label?

There is currently to a PR to remove the v7 and v8 (beta) theme versions within the application but it is getting stale: opensearch-project#978 Since the v8 (beta) theme isn't actually planned and has the incorrect version, we do not want end users to be able to change this setting. In the essence of time, this will prevent this setting from showing in the Advanced Settings page but will also not break users who set this version already (however it will lock them into theme until they switched by manually updating or deleting the config doc). Removing this setting will force the default to be v8 (beta). Temporary fix for: opensearch-project#494 But it should be removed completely. Signed-off-by: Kawika Avilla <[email protected]>

There is currently to a PR to remove the v7 and v8 (beta) theme versions within the application but it is getting stale: #978 Since the v8 (beta) theme isn't actually planned and has the incorrect version, we do not want end users to be able to change this setting. In the essence of time, this will prevent this setting from showing in the Advanced Settings page but will also not break users who set this version already (however it will lock them into theme until they switched by manually updating or deleting the config doc). Removing this setting will force the default to be v8 (beta). Temporary fix for: #494 But it should be removed completely. Signed-off-by: Kawika Avilla <[email protected]>

kavilla · 2022-05-19T19:56:30Z

Will close and link to the issue we did a temp work around for 2.0 but will need to remove this for 3.0

There is currently to a PR to remove the v7 and v8 (beta) theme versions within the application but it is getting stale: #978 Since the v8 (beta) theme isn't actually planned and has the incorrect version, we do not want end users to be able to change this setting. In the essence of time, this will prevent this setting from showing in the Advanced Settings page but will also not break users who set this version already (however it will lock them into theme until they switched by manually updating or deleting the config doc). Removing this setting will force the default to be v8 (beta). Temporary fix for: #494 But it should be removed completely. Signed-off-by: Kawika Avilla <[email protected]> (cherry picked from commit 87e5412)

There is currently to a PR to remove the v7 and v8 (beta) theme versions within the application but it is getting stale: #978 Since the v8 (beta) theme isn't actually planned and has the incorrect version, we do not want end users to be able to change this setting. In the essence of time, this will prevent this setting from showing in the Advanced Settings page but will also not break users who set this version already (however it will lock them into theme until they switched by manually updating or deleting the config doc). Removing this setting will force the default to be v8 (beta). Temporary fix for: #494 But it should be removed completely. Signed-off-by: Kawika Avilla <[email protected]> (cherry picked from commit 87e5412) Co-authored-by: Kawika Avilla <[email protected]>

…t#1598) There is currently to a PR to remove the v7 and v8 (beta) theme versions within the application but it is getting stale: opensearch-project#978 Since the v8 (beta) theme isn't actually planned and has the incorrect version, we do not want end users to be able to change this setting. In the essence of time, this will prevent this setting from showing in the Advanced Settings page but will also not break users who set this version already (however it will lock them into theme until they switched by manually updating or deleting the config doc). Removing this setting will force the default to be v8 (beta). Temporary fix for: opensearch-project#494 But it should be removed completely. Signed-off-by: Kawika Avilla <[email protected]>

zuochengding requested review from tmarkley, kavilla, boktorbb and ananzh November 24, 2021 01:59

zuochengding requested a review from a team as a code owner November 24, 2021 01:59

zuochengding self-assigned this Nov 24, 2021

zuochengding added the v1.3.0 label Nov 24, 2021

kavilla linked an issue Nov 24, 2021 that may be closed by this pull request

[BUG] Theme version references v7 and v8 #494

Open

zuochengding force-pushed the theme-version branch from eeba9e7 to 3e84c22 Compare November 24, 2021 18:40

kavilla previously approved these changes Nov 24, 2021

View reviewed changes

ananzh previously approved these changes Nov 29, 2021

View reviewed changes

zuochengding dismissed stale reviews from ananzh and kavilla via 5f5ebbb November 30, 2021 19:20

zuochengding force-pushed the theme-version branch from 3e84c22 to 5f5ebbb Compare November 30, 2021 19:20

tmarkley requested a review from ahopp November 30, 2021 20:38

tmarkley assigned ahopp Dec 6, 2021

boktorbb previously approved these changes Dec 13, 2021

View reviewed changes

tmarkley reviewed Dec 13, 2021

View reviewed changes

src/core/server/ui_settings/settings/theme.test.ts Show resolved Hide resolved

packages/osd-ui-shared-deps/theme.ts Show resolved Hide resolved

zuochengding dismissed boktorbb’s stale review via 63f7994 December 15, 2021 00:54

zuochengding force-pushed the theme-version branch from 5f5ebbb to 63f7994 Compare December 15, 2021 00:54

zuochengding force-pushed the theme-version branch from 56bfda5 to 1e54595 Compare January 12, 2022 00:11

zuochengding force-pushed the theme-version branch from 1e54595 to 9cca40c Compare January 12, 2022 03:10

tmarkley suggested changes Jan 14, 2022

View reviewed changes

tmarkley suggested changes Feb 7, 2022

View reviewed changes

AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022

feat(partition): legend hover options (opensearch-project#978)

acd1339

tmarkley added ux / ui Improvements or additions to user experience, flows, components, UI elements and removed v2.0.0 labels Apr 14, 2022

kavilla mentioned this pull request May 18, 2022

[Purify] hide option for theme version in settings #1598

Merged

7 tasks

tmarkley unassigned ahopp and zuochengding May 19, 2022

kavilla added the v3.0.0 label May 19, 2022

kavilla closed this May 19, 2022

kavilla mentioned this pull request May 19, 2022

[BUG] Theme version references v7 and v8 #494

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix opensearch theme version #978

Fix opensearch theme version #978

zuochengding commented Nov 24, 2021 •

edited

Loading

kavilla commented Nov 24, 2021

kavilla commented Nov 24, 2021 •

edited

Loading

zuochengding commented Nov 24, 2021 •

edited

Loading

ananzh left a comment

tmarkley commented Nov 29, 2021

jcgraybill commented Nov 30, 2021

ahopp commented Dec 9, 2021

zuochengding commented Dec 9, 2021

ahopp commented Dec 9, 2021

kavilla commented Dec 13, 2021

boktorbb left a comment

zuochengding commented Dec 14, 2021

zuochengding commented Jan 11, 2022

tmarkley left a comment

tmarkley Jan 14, 2022

zuochengding Jan 14, 2022 •

edited

Loading

tmarkley Jan 14, 2022

zuochengding Jan 14, 2022 •

edited

Loading

zuochengding commented Jan 14, 2022 •

edited

Loading

kavilla commented Feb 7, 2022

tmarkley Feb 7, 2022

tmarkley Feb 7, 2022

tmarkley commented Feb 7, 2022

kavilla commented Apr 14, 2022

kavilla commented May 19, 2022

		expect(() => validate('v7')).not.toThrow();
		expect(() => validate('v8 (beta)')).not.toThrow();

Fix opensearch theme version #978

Fix opensearch theme version #978

Conversation

zuochengding commented Nov 24, 2021 • edited Loading

Description

Issues Resolved

Check List

kavilla commented Nov 24, 2021

kavilla commented Nov 24, 2021 • edited Loading

zuochengding commented Nov 24, 2021 • edited Loading

ananzh left a comment

Choose a reason for hiding this comment

tmarkley commented Nov 29, 2021

jcgraybill commented Nov 30, 2021

ahopp commented Dec 9, 2021

zuochengding commented Dec 9, 2021

ahopp commented Dec 9, 2021

kavilla commented Dec 13, 2021

boktorbb left a comment

Choose a reason for hiding this comment

zuochengding commented Dec 14, 2021

zuochengding commented Jan 11, 2022

tmarkley left a comment

Choose a reason for hiding this comment

tmarkley Jan 14, 2022

Choose a reason for hiding this comment

zuochengding Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

tmarkley Jan 14, 2022

Choose a reason for hiding this comment

zuochengding Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

zuochengding commented Jan 14, 2022 • edited Loading

kavilla commented Feb 7, 2022

tmarkley Feb 7, 2022

Choose a reason for hiding this comment

tmarkley Feb 7, 2022

Choose a reason for hiding this comment

tmarkley commented Feb 7, 2022

kavilla commented Apr 14, 2022

kavilla commented May 19, 2022

zuochengding commented Nov 24, 2021 •

edited

Loading

kavilla commented Nov 24, 2021 •

edited

Loading

zuochengding commented Nov 24, 2021 •

edited

Loading

zuochengding Jan 14, 2022 •

edited

Loading

zuochengding Jan 14, 2022 •

edited

Loading

zuochengding commented Jan 14, 2022 •

edited

Loading